Attempt to fix issues seemingly caused by global loading state not being observed. #11392

rtibbles · 2023-10-11T23:40:27Z

Summary

The removal of the global loader from CoreBase was never fully accounted for in the coach reports, which were still relying on the delay of component render until page specific state was instantiated in order to prevent access to potentially undefined state
Fixes this by adding in a temporary measure into the CoachImmersivePage to show nothing except the ImmersivePage linear loader while global loading state is active.
Similar issues seem to be active on the class routes for the Learn plugin, it fixes this by adding in loading states on each individual class route page.

References

Reviewer guidance

@pcenov I wasn't able to exactly replicate the errors you were seeing, but have definitely made the described flow more robust. I think the errors were occurring due to conflicting state being displayed/loaded, but I can't be sure.

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

…ing observed.

github-actions · 2023-10-12T00:33:30Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b6.dev0_git.160.g9f1e04e8.pex
Windows Installer (EXE)	kolibri-0.16.0b6.dev0+git.160.g9f1e04e8-unsigned.exe
Debian Package	kolibri_0.16.0b6.dev0+git.160.g9f1e04e8-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b6.dev0+git.160.g9f1e04e8-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b6.dev0+git.160.g9f1e04e8-0.1.0-debug.apk
TAR file	kolibri-0.16.0b6.dev0+git.160.g9f1e04e8.tar.gz
WHL file	kolibri-0.16.0b6.dev0+git.160.g9f1e04e8-py2.py3-none-any.whl

pcenov

Hi @rtibbles I confirm that you have managed to fix the issue at Coach > Class home however this is still happening at the learner's home page (apologies for not pointing out this more clearly in #11380):

2023-10-12_17-20-51.mp4

rtibbles · 2023-10-12T16:28:52Z

Ah, sorry - I had thought this was only impacting the coach - I will revisit!

…omponent.

rtibbles · 2023-10-12T23:49:14Z

@pcenov I replicated this issue as well and am fairly sure it should now be fixed.

MisRob · 2023-10-13T14:12:20Z

kolibri/plugins/coach/assets/src/views/CoachImmersivePage.vue

@@ -12,7 +12,12 @@
      :loading="loading"
      :appearanceOverrides="appearanceOverrides"
    >
-      <div class="coach-main">
+      <KCircularLoader
+        v-if="coreLoading"


Some time ago, we attempted to move away from having global loading state being managed by these root pages but instead let it be managed by the child pages as such architecture showed up not being very flexible and on some cases preventing us from updating pages in a reasonable manner. I'm concerned that by introducing this again here will lead to same issues. I didn't dig into detail into all pages at this point, but here's one example where I see potential confusion https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/coach/assets/src/views/plan/QuizEditDetailsPage.vue#L93-L96

I didn't review in detail yet and can think about it more. Just looking at it very high-level, I think that this type of change may easily end up having unexpected impact and seems to go against the new pattern we've tried to adopt in the whole codebase to address problems caused by this approach. For now, I wanted to ask what was the reason for introducing it in this manner and if you've already considered alternatives.

I know it'd be lots of work to do it from child pages. On the other hand, it seems that we'd need to have a look at them anyways to cleanup conflicting or overlapping loading states so that we don't run again into issues we've been cleaning up lately after the core base refactor.

I did this because when the refactor happened to remove the global loading state from CoreBase, it wasn't then re-added to each individual page in coach, which for many of the pages were reliant on the global loading state preventing their components from being shown until state has been set.

As a result, all of these components make unguarded reads from their module state - or are not set up to reactively update if the module state changes while they are displayed. This is the cause of the bug that Peter has reported here.

I agree that we should be moving in the direction you describe, but this is a bandaid to account for the incomplete refactor, and I think not doing it is far more likely to leave additional regressions undiscovered until after release.

The code you are pointing to has not changed for 4 years (long predating the refactor) so I think the chance of this change causing issues for the loading state there is minimal.

I definitely agree it's caused by incomplete refactor and no problem to keep addressing this outside of the scope of this particular PR. My only suggestion would be to have an issue explaining why this change happened and in what direction we want to move on.

The code you are pointing to has not changed for 4 years (long predating the refactor) so I think the chance of this change causing issues for the loading state there is minimal.

I would rather say that we don't know, but one thing we can do to be more sure is to test loading states thoroughly.

One last high-level question, before I look into details, why is circular loader used rather than the linear one?

The only PRs that I am aware of that have made changes to the loading state since the refactor are this one: #11218 and this one: #11201

Neither of which affect pages that use the CoachImmersivePage, so I think the chance of a conflict between this change and previous loading state work. Unless I've missed a PR?

I can also raise the issue of global vs local loaders in a KDS tactical if that'd be helpful. I heard more devs "complaining" about it :)

I am fine with mostly doing away with global loaders (although I think having the linear loader consistently be turned on until route navigation has completed at least would be fairly easy to do globally, without having to mess around with anything in each route) - but getting rid of them completely would mean that we need to rearchitect all our pages to handle their loading states internally (which is probably best achieved by moving their loading state into setup in the component).

although I think having the linear loader consistently be turned on until route navigation has completed at least would be fairly easy to do globally, without having to mess around with anything in each route

Yes, I see value in this. Can't think of any issues in this particular case. I assume it should be pretty brief (after we deal with issues like #11219) so in that case it may even make more sense to stay with the subtler global linear loader.

By "it" I meant route navigation, if we stop doing API requests there

pcenov

@rtibbles Now when I submit the quiz I am brought back to the Home page seeing only the spinner icon and manually refreshing the page doesn't help. I have to navigate to a different plugin and come back to the Home page to see the correct data:

2023-10-16_14-33-17.mp4

rtibbles · 2023-10-16T16:18:34Z

Odd, I'll take another look @pcenov.

Use loading prop instead.

rtibbles · 2023-10-16T18:15:03Z

Hi @pcenov - thanks, this was again caused by some sloppy programming by me - this should now be resolved (the error was that it was always waiting about 30 seconds to stop showing the loading state!)

pcenov

Thanks @rtibbles - everything is working correctly now, good to go!

MisRob

Thank you @rtibbles. In the light of what we discussed before, I think it's good to go. I didn't check in detail the affected sub-pages - relying on QA done. One thing I'd ask you is to only prepare a follow-up issue we talked about earlier.

rtibbles · 2023-10-17T14:36:43Z

Follow up filed here: #11422

MisRob · 2023-10-17T15:27:35Z

Thank you, it's looking great

Attempt to fix issues seemingly caused by global loading state not be…

704a31d

…ing observed.

rtibbles added the TODO: needs review Waiting for review label Oct 11, 2023

rtibbles added this to the Kolibri 0.16 Release: Bugs and Regressions milestone Oct 11, 2023

rtibbles requested review from marcellamaki and pcenov October 11, 2023 23:40

github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: very small labels Oct 11, 2023

pcenov requested changes Oct 12, 2023

View reviewed changes

Adds loaders to all class route page components or nearest relevant c…

ab65f72

…omponent.

rtibbles requested a review from MisRob October 12, 2023 23:48

github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small labels Oct 12, 2023

pcenov self-requested a review October 13, 2023 06:30

MisRob reviewed Oct 13, 2023

View reviewed changes

Remove KCircularLoader to conform to loader standards.

50e1295

pcenov requested changes Oct 16, 2023

View reviewed changes

Revert addition of loading state that waited for long timeout.

782aa60

Use loading prop instead.

pcenov self-requested a review October 17, 2023 10:36

pcenov approved these changes Oct 17, 2023

View reviewed changes

MisRob approved these changes Oct 17, 2023

View reviewed changes

rtibbles mentioned this pull request Oct 17, 2023

Implement individual conditional loading behaviour in every page that uses the CoachImmersivePage #11422

Open

rtibbles merged commit e1b6d2c into learningequality:release-v0.16.x Oct 17, 2023
34 checks passed

rtibbles deleted the quiz_score_report branch October 17, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to fix issues seemingly caused by global loading state not being observed. #11392

Attempt to fix issues seemingly caused by global loading state not being observed. #11392

rtibbles commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 12, 2023 •

edited

Loading

pcenov left a comment

rtibbles commented Oct 12, 2023

rtibbles commented Oct 12, 2023

MisRob Oct 13, 2023 •

edited

Loading

MisRob Oct 13, 2023

rtibbles Oct 13, 2023

MisRob Oct 13, 2023

rtibbles Oct 13, 2023

rtibbles Oct 13, 2023

MisRob Oct 13, 2023 •

edited

Loading

rtibbles Oct 13, 2023 •

edited

Loading

MisRob Oct 16, 2023

MisRob Oct 16, 2023

pcenov left a comment

rtibbles commented Oct 16, 2023

rtibbles commented Oct 16, 2023

pcenov left a comment

MisRob left a comment

rtibbles commented Oct 17, 2023

MisRob commented Oct 17, 2023

Attempt to fix issues seemingly caused by global loading state not being observed. #11392

Attempt to fix issues seemingly caused by global loading state not being observed. #11392

Conversation

rtibbles commented Oct 11, 2023 • edited Loading

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Oct 12, 2023 • edited Loading

pcenov left a comment

Choose a reason for hiding this comment

rtibbles commented Oct 12, 2023

rtibbles commented Oct 12, 2023

MisRob Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

rtibbles Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcenov left a comment

Choose a reason for hiding this comment

rtibbles commented Oct 16, 2023

rtibbles commented Oct 16, 2023

pcenov left a comment

Choose a reason for hiding this comment

MisRob left a comment

Choose a reason for hiding this comment

rtibbles commented Oct 17, 2023

MisRob commented Oct 17, 2023

rtibbles commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 12, 2023 •

edited

Loading

MisRob Oct 13, 2023 •

edited

Loading

MisRob Oct 13, 2023 •

edited

Loading

rtibbles Oct 13, 2023 •

edited

Loading